-
-
Notifications
You must be signed in to change notification settings - Fork 2
Fix calling the shoul function #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes update the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Policies/ControlledPolicy.php (1)
10-10: Consider addressing the TODO comment.There's an existing TODO comment about handling additional methods like attach/restore/force_delete, but the code already implements restore and forceDelete methods. Consider updating or removing this comment to reflect the current implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Policies/ControlledPolicy.php(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Policies/ControlledPolicy.php (1)
src/Controls/Control.php (1)
applies(49-68)
🔇 Additional comments (3)
src/Policies/ControlledPolicy.php (3)
48-48: Appropriate method replacement.Replacing
shouldwithappliesis the correct approach. Looking at theappliesmethod from the Control class, it handles the permission check more comprehensively by iterating through perimeters and properly handling model existence.
61-61: Consistent implementation across policy methods.All policy methods have been consistently updated to use the
appliesmethod with the same parameter structure. This ensures uniform behavior across all access control operations.Also applies to: 86-86, 99-99, 112-112, 125-125
73-73: Correct handling for creation permissions.The
createmethod appropriately usesapplieswith a new model instance, which allows the permission system to check if the user can create models of this type without requiring an existing instance.
It was impossible to call the should function from controlledPolicy. I changed this by calling applies
Summary by CodeRabbit